[libc++] Remove assertions from <string_view> that are unreachable #148598
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesWhen assertions are enabled it is impossible to construct a Full diff: https://github.com/llvm/llvm-project/pull/148598.diff 1 Files Affected:
diff --git a/libcxx/include/string_view b/libcxx/include/string_view
index 861187c0640e1..4a85c73733023 100644
--- a/libcxx/include/string_view
+++ b/libcxx/include/string_view
@@ -500,7 +500,6 @@ public:
// find
_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI size_type
find(basic_string_view __s, size_type __pos = 0) const _NOEXCEPT {
- _LIBCPP_ASSERT_NON_NULL(__s.size() == 0 || __s.data() != nullptr, "string_view::find(): received nullptr");
return std::__str_find<value_type, size_type, traits_type, npos>(data(), size(), __s.data(), __pos, __s.size());
}
@@ -524,7 +523,6 @@ public:
// rfind
_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI size_type
rfind(basic_string_view __s, size_type __pos = npos) const _NOEXCEPT {
- _LIBCPP_ASSERT_NON_NULL(__s.size() == 0 || __s.data() != nullptr, "string_view::find(): received nullptr");
return std::__str_rfind<value_type, size_type, traits_type, npos>(data(), size(), __s.data(), __pos, __s.size());
}
@@ -549,7 +547,6 @@ public:
// find_first_of
_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI size_type
find_first_of(basic_string_view __s, size_type __pos = 0) const _NOEXCEPT {
- _LIBCPP_ASSERT_NON_NULL(__s.size() == 0 || __s.data() != nullptr, "string_view::find_first_of(): received nullptr");
return std::__str_find_first_of<value_type, size_type, traits_type, npos>(
data(), size(), __s.data(), __pos, __s.size());
}
@@ -575,7 +572,6 @@ public:
// find_last_of
_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI size_type
find_last_of(basic_string_view __s, size_type __pos = npos) const _NOEXCEPT {
- _LIBCPP_ASSERT_NON_NULL(__s.size() == 0 || __s.data() != nullptr, "string_view::find_last_of(): received nullptr");
return std::__str_find_last_of<value_type, size_type, traits_type, npos>(
data(), size(), __s.data(), __pos, __s.size());
}
@@ -601,8 +597,6 @@ public:
// find_first_not_of
_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI size_type
find_first_not_of(basic_string_view __s, size_type __pos = 0) const _NOEXCEPT {
- _LIBCPP_ASSERT_NON_NULL(
- __s.size() == 0 || __s.data() != nullptr, "string_view::find_first_not_of(): received nullptr");
return std::__str_find_first_not_of<value_type, size_type, traits_type, npos>(
data(), size(), __s.data(), __pos, __s.size());
}
@@ -628,8 +622,6 @@ public:
// find_last_not_of
_LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI size_type
find_last_not_of(basic_string_view __s, size_type __pos = npos) const _NOEXCEPT {
- _LIBCPP_ASSERT_NON_NULL(
- __s.size() == 0 || __s.data() != nullptr, "string_view::find_last_not_of(): received nullptr");
return std::__str_find_last_not_of<value_type, size_type, traits_type, npos>(
data(), size(), __s.data(), __pos, __s.size());
}
|
|
I'd like @var-const to have a look cause we did encounter some issues with this check in |
|
Thanks for pinging! Sorry I'm just now getting to this. @ldionne I believe the issues you probably have in mind were in @philnik777 It's not 100% true that these are unreachable: https://godbolt.org/z/7sxTez18q. I can't recall why the constructor check is not enabled in the C++11 mode (it seems to ring a very vague bell, but maybe I'm confusing it with something). Doing a little archaeology, it seems like it was that way from the very first implementation by Marshal. @ldionne Do you have any recollection on this, perhaps? I'd be on board with removing the version check in the constructor's assertion (assuming there's no good reason to keep it, at least now) which will clear the way for the change in this patch. But perhaps there's a reason to keep the version check? |
|
@var-const This was most likely done because C++11 didn't allow anything in the constructor body in C++11. Clang accepts it as an extension now, so we can simply enable the asserts in C++11 mode for Clang. |
SGTM, can you please do it in this patch as well? |
philnik777
force-pushed
the
string_view_remove_asserts
branch
from
b6c6104 to
d832e1f
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@var-const are you happy with this now? |
There was a problem hiding this comment.
LGTM with minor comments. I spoke with @var-const just now and he was fine with the patch as well (but can chime in if needed).
philnik777
force-pushed
the
string_view_remove_asserts
branch
from
d832e1f to
3897132
Compare
When assertions are enabled it is impossible to construct a
string_viewwhich contains a null pointer and a non-zero size, so assertions where we check for that on an already constructedstring_vieware unreachable.